Transcode image format#102
Conversation
|
Thank you @cargocultprogramming ! I will take a bit more time to review the code, but here are some thoughts from my first pass review; please comment if any of these things are proving to be frustrating to resolve on your side.
|
patrickfournier
left a comment
There was a problem hiding this comment.
Thanks for your work, this is very much appreciated.
Code
Overall, the implementation looks good. I left a few comments, mostly related to falling back to a default setting for the picture transform type. Maybe it was not clear from my comment in the related issue, but I was thinking the default output-format could be used with all transform types. So for pictures, we would have:
IMAGE_PROCESS = {
"example-pict": {
"type": "picture",
"output-format": "webp", # Default format
"sources": [
{
"name": "default",
"media": "(min-width: 640px)",
"srcset": [
("640w", ["scale_in 640 480 True"]),
("1024w", ["scale_in 1024 683 True"]),
("1600w", ["scale_in 1600 1200 True"]),
],
"sizes": "100vw",
},
{
"name": "source-1",
"srcset": [
("1x", ["crop 100 100 200 200"]),
("2x", ["crop 100 100 300 300"]),
]
},
],
"default": ("default", "640w"),
},
}
Tests
Please test all possible syntaxes:
- Image replacement: please add a test for the short syntax (
"article-image": (["scale_in 300 300 True"], 'webp') . - Responsive image: please add a test to check proper behavior when
output-formatis not specified. - Picture set: please add a test that uses the default
output-format - For responsive images and/or picture, it would be nice to have a test for the case where
defaultspecifies a transform instead of a name, for example:"default": (["scale_in 1600 1200 True"], "webp")instead of"default": "800w"
Documentation
As @minchinweb mentioned, please update the README to document the feature and the new syntax.
|
I've added a number of commits today (1e959d5 to e22ee04), to address the comments of @minchinweb :
The tests that I added are:
@patrickfournier thanks for the code comments, I'll address those later, I'm all out of steam for today. |
…rns None as requested.
|
@patrickfournier thank you again for your comments. I hopefully addressed everything - see below.
I've added this syntax to the tests.
Added a test for this. This case was actually not covered, so I added this functionality.
Test added.
Test added.
Test added. This case was also not covered yet in the code, but i'ts supported now.
Already covered in earlier commits. Regarding your code comments / change requests - I think I covered everything in the last couple of commits. There is a fallback now when get_target_format() returns None, which was the major point. I commented the other points directly. |
patrickfournier
left a comment
There was a problem hiding this comment.
Thanks for the fixes and the doc.
Unless I am mistaken, I think there are still two cases where we do not correctly fallback on output-format.
I also added a request for change in the test code, and a few corrections to the README. English is not my native language, so someone else should review this as well.
Co-authored-by: Patrick Fournier <patrick@patrickfournier.com>
Thanks for your comments. I've added an explicit test for the fallback in 22f49c6 (making sure it properly fails before the fix) and hopefully resolved the issue in fe32ae1.
The test code was indeed overly complicated, i changed it to test against a simple dict instead of the complex function in 7cad4aa.
Obviously it's not my native language either, afaics your proposed changes are all sound and i accepted them. |
Pull request for #101
Feature implemented with new configuration options as proposed by @patrickfournier in #101 (comment).
The implementation of the feature caused two functions to get too long for the linter, so I had to refactor them into smaller chunks, which is why this PR got a bit heavy even though I made no unrelated changes. I added a new test file to test the format conversion and also did a limited manual test in an actual pelican project, though some more thorough testing probably won't hurt.
The entire setup is backwards-compatible, so if you don't change any settings, you'll end up with the same results as in the earlier version.
Hope this is useful - feel free to comment and change whatever needs commenting and changing.